W3C Trace propagator tests - switch to level 1 #4937

Kielek · 2023-10-09T07:30:13Z

~~Fixes test_tracestate_duplicated_keys from w3c test suite~~

~~Follow up to #4893, there will be one more PR to fix random flags.~~

~~Ref: https://github.com/w3c/trace-context/blob/551a5b0855171281e98b4c2a814bf9e1f837cd53/test/test.py#L565-L567~~

Changes

~~Duplicated keys are fully propagated now. It allows to drop creating HashSet for each propagation execution. Now only counter is created.~~

~~Opposite option, propagate only one value for given key. I think that it requires keeping HashSet.~~

Switch W3C Trace tests to level 1, based on: #4937 (comment)

Merge requirement checklist

CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
Unit tests added/updated
~~[ ] Appropriate CHANGELOG.md files updated for non-trivial changes~~
~~[ ] Changes in public API reviewed (if applicable)~~

codecov · 2023-10-09T07:48:32Z

Codecov Report

Merging #4937 (904935a) into main (d1d6d4b) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4937      +/-   ##
==========================================
- Coverage   83.42%   83.39%   -0.04%     
==========================================
  Files         296      296              
  Lines       12377    12377              
==========================================
- Hits        10326    10322       -4     
- Misses       2051     2055       +4

Flag	Coverage Δ
unittests	`83.39% <ø> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 7 files with indirect coverage changes

utpilla · 2023-10-10T04:37:08Z

src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs

@@ -294,19 +294,13 @@ internal static bool TryExtractTracestate(string[] tracestateCollection, out str
                        return false;
                    }

-                    // ValidateKey() call above has ensured the key does not contain upper case letters.
-                    if (!keySet.Add(key.ToString()))


https://github.com/w3c/trace-context/blob/551a5b0855171281e98b4c2a814bf9e1f837cd53/test/test.py#L565-L567

expects the tracestate to be inherited, and the duplicated keys to be either kept as-is or one of them to be discarded

Could we discard the duplicate ones instead of keeping them? We are only allowed 32 keys. Why waste them by storing duplicates when we could make room for other unique keys?

I have tried to answer in PR description on it what was the reason for the change:

Duplicated keys are fully propagated now. It allows to drop creating HashSet for each propagation execution. Now only counter is created.

If you think that it is worth to keep the checj, I can update this PR.

On second thought, I think we could just keep the key as-is in favor of performance as this would be exectued in the hot-path.

@alanwest @CodeBlanch @vishweshbankwar It would be great to know your thoughts as well.

I am good to keep this as spec allows it.
Not for this PR - It would be good to check how other languages are doing it (would be good to keep consistent behavior)

Java is doing similar things, at the beginning checking if there is more/less than 32 attributes https://github.com/open-telemetry/opentelemetry-java/blob/57d83344178f578eb5d2f043b0ffc5c42b6719a5/api/all/src/main/java/io/opentelemetry/api/trace/propagation/internal/W3CTraceContextEncoding.java#L43-L45

Go-lang has similar issue like here, before changes: https://github.com/open-telemetry/opentelemetry-go/blob/c047088605b454f172765621d53107f80b1e6417/trace/tracestate.go#L123-L126

Python is checking duplicates: https://github.com/open-telemetry/opentelemetry-python/blob/34d11d59e9b37052eb3aa6706288641ecb3056e7/opentelemetry-api/src/opentelemetry/trace/span.py#L231-L240

I do not think that there we will be able to find common solution.

pellared · 2023-10-16T16:33:16Z

Cross-posting open-telemetry/opentelemetry-go#4638 (comment)

alanwest · 2023-10-16T23:52:01Z

test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs

+        Assert.Equal("foo=1,foo=1", CallTraceContextPropagator("foo=1,foo=1"));
+        Assert.Equal("foo=1,foo=2", CallTraceContextPropagator("foo=1,foo=2"));


Is this the correct behavior or is the behavior in @pellared's PR for opentelemetry-go correct? That is:

Assert.Equal("foo=1,foo=1", CallTraceContextPropagator("foo=1")); Assert.Equal("foo=1,foo=2", CallTraceContextPropagator("foo=1"));

Ok, nevermind. After speaking with @utpilla, sounds like both behaviors are correct and left up to the implementation.

alanwest · 2023-10-16T23:54:39Z

Cross-posting open-telemetry/opentelemetry-go#4638 (comment)

Not sure I understand... should this PR become a draft too because the W3C spec has not yet clarified the behavior?

pellared · 2023-10-17T06:01:59Z

Cross-posting open-telemetry/opentelemetry-go#4638 (comment)

Not sure I understand... should this PR become a draft too because the W3C spec has not yet clarified the behavior?

Also because W3C spec is not released.

utpilla · 2023-10-17T20:55:14Z

Cross-posting open-telemetry/opentelemetry-go#4638 (comment)

Not sure I understand... should this PR become a draft too because the W3C spec has not yet clarified the behavior?

Also because W3C spec is not released.

@pellared I don't follow. Are there any specific links that you're looking at to determine the readiness of the W3C spec? We are trying to find out if it's okay for us to merge the changes proposed in this PR.

pellared · 2023-10-17T21:34:26Z

Cross-posting open-telemetry/opentelemetry-go#4638 (comment)

Not sure I understand... should this PR become a draft too because the W3C spec has not yet clarified the behavior?

Also because W3C spec is not released.

@pellared I don't follow. Are there any specific links that you're looking at to determine the readiness of the W3C spec? We are trying to find out if it's okay for us to merge the changes proposed in this PR.

The changes from Specify uniqueness of tracestate keys as the sender's responsibility w3c/trace-context#477 are not part of the released W3C Trace stable release. See https://www.w3.org/TR/trace-context/#combined-header-value vs https://w3c.github.io/trace-context/#combined-header-value. They are only in the draft.
I am not sure even certain if the specification clearly defines the changes proposed in this PR. I created Document handling of tracestate duplicate keys w3c/trace-context#548

CC @reyang

Kielek · 2023-10-18T04:42:06Z

I will try to add more comment here.

When the docker test image the latest commit from

opentelemetry-dotnet/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/Dockerfile

Line 12 in 1ec7a63

RUN git clone https://github.com/w3c/trace-context.git

is downloaded. Then the OTel implementation of propagator is executed against latest changes from https://github.com/w3c/trace-context repository.

There are 2 versions of the specification:

W3C Recommendation 23 November 2021, Level 1: https://www.w3.org/TR/trace-context/#combined-header-value release
W3C Candidate Recommendation Snapshot 18 April 2023, Level 2: https://www.w3.org/TR/trace-context-2/#combined-header-value

Changes in tests w3c/trace-context#477 was merged in 2022, so after the Level 1 was released.

I see two options to proceed here:

Start executing tests on Level 1 whish is stable.
Keep executing tests for Level 2 which is in lets say RC phase.

For both cases test suited should be fetched from exact commit instead of the latest main.

utpilla · 2023-10-19T18:12:57Z

I will try to add more comment here.

When the docker test image the latest commit from

opentelemetry-dotnet/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/Dockerfile

Line 12 in 1ec7a63

RUN git clone https://github.com/w3c/trace-context.git

is downloaded. Then the OTel implementation of propagator is executed against latest changes from https://github.com/w3c/trace-context repository.
There are 2 versions of the specification:

W3C Recommendation 23 November 2021, Level 1: https://www.w3.org/TR/trace-context/#combined-header-value release

W3C Candidate Recommendation Snapshot 18 April 2023, Level 2: https://www.w3.org/TR/trace-context-2/#combined-header-value

Changes in tests w3c/trace-context#477 was merged in 2022, so after the Level 1 was released.

I see two options to proceed here:

Start executing tests on Level 1 whish is stable.

Keep executing tests for Level 2 which is in lets say RC phase.

For both cases test suited should be fetched from exact commit instead of the latest main.

I think we should aim for complying with the stable spec.

@alanwest Thoughts?

github-actions · 2023-10-27T03:16:47Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

Kielek · 2023-10-27T11:32:23Z

@utpilla, @alanwest, @vishweshbankwar, please review once more time. The PR is fully changed - tests are now executed based on Level 1 of specification (and passing on .NET7).

See: f6b2d2a

Kielek · 2023-10-27T11:45:58Z

.NET8 failure seems to be unrelated to changes. Please rerun.

test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/Dockerfile

Kielek requested a review from a team October 9, 2023 07:30

Kielek force-pushed the fix-test_tracestate_duplicated_keys branch from 90bde8d to b43bdfb Compare October 9, 2023 07:43

Fix test_tracestate_duplicated_keys

e785a3a

Kielek force-pushed the fix-test_tracestate_duplicated_keys branch from b43bdfb to e785a3a Compare October 9, 2023 07:57

utpilla reviewed Oct 10, 2023

View reviewed changes

utpilla approved these changes Oct 11, 2023

View reviewed changes

Merge branch 'main' into fix-test_tracestate_duplicated_keys

f07f636

vishweshbankwar approved these changes Oct 13, 2023

View reviewed changes

Merge branch 'main' into fix-test_tracestate_duplicated_keys

cb2ecd8

pellared mentioned this pull request Oct 16, 2023

trace: ParseTraceState ignores duplicated tracestate keys open-telemetry/opentelemetry-go#4638

Closed

alanwest reviewed Oct 16, 2023

View reviewed changes

alanwest approved these changes Oct 17, 2023

View reviewed changes

utpilla and others added 3 commits October 16, 2023 20:30

Merge branch 'main' into fix-test_tracestate_duplicated_keys

705cecd

Merge branch 'main' into fix-test_tracestate_duplicated_keys

a4221b2

Update CHANGELOG.md

6788d16

Merge branch 'main' into fix-test_tracestate_duplicated_keys

d7ad2c4

Merge branch 'main' into fix-test_tracestate_duplicated_keys

d1ee774

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 27, 2023

Kielek added 3 commits October 27, 2023 10:15

Merge branch 'main' into fix-test_tracestate_duplicated_keys

2a02548

Merge branch 'main' into fix-test_tracestate_duplicated_keys

b4faa7c

W3Trace propagator tests - switch to level 1

f6b2d2a

Kielek force-pushed the fix-test_tracestate_duplicated_keys branch from c52d2a4 to f6b2d2a Compare October 27, 2023 11:21

Kielek changed the title ~~Fix test_tracestate_duplicated_keys~~ W3Trace propagator tests - switch to level 1 Oct 27, 2023

Kielek changed the title ~~W3Trace propagator tests - switch to level 1~~ W3C Trace propagator tests - switch to level 1 Oct 27, 2023

Kielek requested review from utpilla, vishweshbankwar and alanwest October 27, 2023 11:31

pellared approved these changes Oct 27, 2023

View reviewed changes

utpilla reviewed Oct 27, 2023

View reviewed changes

test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/Dockerfile Show resolved Hide resolved

github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 28, 2023

alanwest approved these changes Oct 31, 2023

View reviewed changes

utpilla approved these changes Nov 1, 2023

View reviewed changes

Merge branch 'main' into fix-test_tracestate_duplicated_keys

904935a

utpilla merged commit ff9912f into open-telemetry:main Nov 1, 2023
35 checks passed

Kielek deleted the fix-test_tracestate_duplicated_keys branch November 1, 2023 10:57

pellared mentioned this pull request Apr 2, 2024

[api-baggage] fix encoding of space chars in baggage item value #5303

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

W3C Trace propagator tests - switch to level 1 #4937

W3C Trace propagator tests - switch to level 1 #4937

Kielek commented Oct 9, 2023 •

edited

Loading

codecov bot commented Oct 9, 2023 •

edited

Loading

utpilla Oct 10, 2023 •

edited

Loading

Kielek Oct 10, 2023 •

edited

Loading

utpilla Oct 11, 2023 •

edited

Loading

vishweshbankwar Oct 13, 2023

Kielek Oct 16, 2023

pellared commented Oct 16, 2023

alanwest Oct 16, 2023 •

edited

Loading

alanwest Oct 17, 2023

alanwest commented Oct 16, 2023

pellared commented Oct 17, 2023

utpilla commented Oct 17, 2023

pellared commented Oct 17, 2023

Kielek commented Oct 18, 2023

utpilla commented Oct 19, 2023

github-actions bot commented Oct 27, 2023

Kielek commented Oct 27, 2023

Kielek commented Oct 27, 2023

		Assert.Equal("foo=1,foo=1", CallTraceContextPropagator("foo=1,foo=1"));
		Assert.Equal("foo=1,foo=2", CallTraceContextPropagator("foo=1,foo=2"));

W3C Trace propagator tests - switch to level 1 #4937

W3C Trace propagator tests - switch to level 1 #4937

Conversation

Kielek commented Oct 9, 2023 • edited Loading

Changes

Merge requirement checklist

codecov bot commented Oct 9, 2023 • edited Loading

Codecov Report

utpilla Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Kielek Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

utpilla Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

vishweshbankwar Oct 13, 2023

Choose a reason for hiding this comment

Kielek Oct 16, 2023

Choose a reason for hiding this comment

pellared commented Oct 16, 2023

alanwest Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

alanwest Oct 17, 2023

Choose a reason for hiding this comment

alanwest commented Oct 16, 2023

pellared commented Oct 17, 2023

utpilla commented Oct 17, 2023

pellared commented Oct 17, 2023

Kielek commented Oct 18, 2023

utpilla commented Oct 19, 2023

github-actions bot commented Oct 27, 2023

Kielek commented Oct 27, 2023

Kielek commented Oct 27, 2023

Kielek commented Oct 9, 2023 •

edited

Loading

codecov bot commented Oct 9, 2023 •

edited

Loading

utpilla Oct 10, 2023 •

edited

Loading

Kielek Oct 10, 2023 •

edited

Loading

utpilla Oct 11, 2023 •

edited

Loading

alanwest Oct 16, 2023 •

edited

Loading